Make RetryState.isLastAttempt private, and simplify the code#1961
Make RetryState.isLastAttempt private, and simplify the code#1961stIncMale merged 8 commits intomongodb:mainfrom
RetryState.isLastAttempt private, and simplify the code#1961Conversation
JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…imit does not conflict with timeout
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| advance(retryState); | ||
| retryState.breakAndThrowIfRetryAnd(() -> false); | ||
| assertFalse(retryState.isLastAttempt()); | ||
| assertAdvanceOrThrow(null, retryState, new RuntimeException()); |
There was a problem hiding this comment.
assertAdvanceOrThrow(null, retryState, new RuntimeException()) isn’t immediately clear at the call site because the meaning of null isn’t obvious.
Let's introduce/rename small helpers like assertAdvanceOrThrowSucceeds(retryState, attemptException) / assertAdvanceOrThrowFails(expectedException, retryState, attemptException) (or similar) so the intent is self-documenting?
There was a problem hiding this comment.
Done in a7d9dfc.
I used ...Throws and ...DoesNotThrow, following JUnit 5 naming.
Turned out, `assertFalse(operationTimeout && attemptLimit)` holds only as long as there is `retryUntilTimeoutThrowsException`, which I am going to remove, because it is useless. There is no reason to add an assertion that is known to be removed soon.
…eptionWhenTransformerSwallowOriginalTimeoutException`
… `RetryStateTest`
With the backpressure spec changes, it has become painfully obvious that the logic of restricting the number of retries based on either a fixed number of retries, or a CSOT exception, is business logic, and not the internal logic of
RetryingSupplier/RetryState. Given that most of the business logic lives outside ofRetryingSupplier/RetryState, the part about the limit will also be moved out. This PR, just like #1952, does the preliminary work.JAVA-5956, JAVA-6117, JAVA-6113, JAVA-6119, JAVA-6141